[Perf] Cache PropertyChanged/ChangingEventArgs on BindableProperty - #34136
[Perf] Cache PropertyChanged/ChangingEventArgs on BindableProperty#34136simonrozsival wants to merge 4 commits into
Conversation
simonrozsival
added
perf/general
fbd80fc to
37f3a7e
Compare
|
🚀 Dogfood this PR with:
curl -fsSL https://raw.githubusercontent.com/dotnet/maui/main/eng/scripts/get-maui-pr.sh | bash -s -- 34136Or
iex "& { $(irm https://raw.githubusercontent.com/dotnet/maui/main/eng/scripts/get-maui-pr.ps1) } 34136" |
simonrozsival
changed the title
There was a problem hiding this comment.
Pull request overview
This PR targets allocation/performance hot paths in BindableObject.SetValue/ClearValue by caching PropertyChangedEventArgs and PropertyChangingEventArgs per BindableProperty, so repeated property updates don’t allocate new event-args instances.
Changes:
- Add lazy cached
PropertyChangedEventArgs/PropertyChangingEventArgsinstances onBindableProperty. - Add new
BindableObjectoverloads that raise events using pre-allocated event args. - Update
SetValueActual,OnBindablePropertySet, andClearValueCoreto use cached event args.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| src/Controls/src/Core/BindableProperty.cs | Adds lazy per-property cached event-args accessors. |
| src/Controls/src/Core/BindableObject.cs | Uses cached event args via new overloads during SetValue/ClearValue event raising. |
| { | ||
| property.PropertyChanging?.Invoke(this, original.Value, newValue); | ||
| OnPropertyChanging(property.PropertyName); | ||
| OnPropertyChanging(property.CachedPropertyChangingEventArgs); |
There was a problem hiding this comment.
These calls now use the new internal overload OnPropertyChanging(PropertyChangingEventArgs) instead of the existing protected virtual OnPropertyChanging(string). That bypasses overrides in derived types (e.g., Cell.OnPropertyChanging) and can change behavior during SetValue/ClearValue. Keep invoking the virtual OnPropertyChanging(string) hook (or refactor so the cached args path still routes through the virtual method) so derived behavior remains intact while avoiding allocations.
| OnPropertyChanging(property.CachedPropertyChangingEventArgs); | |
| OnPropertyChanging(property.PropertyName); |
| property.PropertyChanging?.Invoke(this, original, value); | ||
|
|
||
| OnPropertyChanging(property.PropertyName); | ||
| OnPropertyChanging(property.CachedPropertyChangingEventArgs); |
There was a problem hiding this comment.
Using OnPropertyChanging(property.CachedPropertyChangingEventArgs) calls the new non-virtual overload and bypasses protected virtual OnPropertyChanging(string) overrides in derived classes. This is a behavioral breaking change (e.g., Cell relies on OnPropertyChanging("Parent")). The allocation optimization needs to preserve the virtual hook; consider routing cached args through the existing virtual path (or introducing a new virtual core method that derived types can override without breaking existing overrides).
| OnPropertyChanging(property.CachedPropertyChangingEventArgs); | |
| OnPropertyChanging(property.PropertyName); |
| if (willFirePropertyChanged) | ||
| { | ||
| OnPropertyChanged(property.PropertyName); | ||
| OnPropertyChanged(property.CachedPropertyChangedEventArgs); |
There was a problem hiding this comment.
OnBindablePropertySet now calls the new internal OnPropertyChanged(PropertyChangedEventArgs) overload, which directly raises PropertyChanged and bypasses the existing protected virtual OnPropertyChanged(string) hook. Many controls (notably Element) override OnPropertyChanged(string) for important behavior (effects, handler updates), so this changes runtime semantics and will likely break tests. The cached-args optimization should still invoke the virtual OnPropertyChanged(string) override chain (or otherwise preserve that extensibility point) while avoiding per-call allocations.
| OnPropertyChanged(property.CachedPropertyChangedEventArgs); | |
| OnPropertyChanged(property.PropertyName); |
…bleProperty Fixes #34092 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
… caching The Element.cs change (_currentPropertyBeingSet tracking) was fragile. Keep the core optimization: cache PropertyChanged/ChangingEventArgs on BindableProperty and use them from BindableObject.SetValueCore. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
8697409 to
896c058
Compare
The previous implementation bypassed OnPropertyChanged/OnPropertyChanging virtual overrides in 20+ derived classes. This fix moves the caching to static dictionaries keyed by property name, and uses them inside the existing virtual methods so derived class overrides are still called. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
@mattleibow @StephaneDelcroix could you please have a look at this PR and let me know what you think about it? could we merge this into net11.0 soon? |
|
|
||
| internal ValidateValueDelegate ValidateValue { get; private set; } | ||
|
|
||
| private static readonly ConcurrentDictionary<string, PropertyChangedEventArgs> s_changedArgsCache = new(); |
There was a problem hiding this comment.
I believe we should set up a max value here. For Maui properties, this can be fixed size and will not cause any memory pressure, but for apps, the large ones mainly, with custom controls and different property names, it can cause a memory pressure. In the wild, I can say that people choose funny names for their properties; for example, instead of choosing Text as a property name, it can be Title, TitleText, etc., causing this collection to grow and may cause a memory pressure at some point.
|
/review -b feature/refactor-copilot-yml |
This comment has been minimized.
This comment has been minimized.
AI code review for net11.0 targetVerdict: Needs discussion (one subtle behavior change to confirm; otherwise a clean, well-scoped perf change) This is an independent review (diff-first, then reconciled with the PR narrative). It is not an approval — a human still needs to sign off. What the PR doesCaches Findings
CIAll required Confidence: high on the analysis; the only open item is the |
kubaflo
left a comment
kubaflo
left a comment
There was a problem hiding this comment.
NEEDS_DISCUSSION (confidence: medium) — multi-model review (3 ND, 1 NC); CI green.
This is a clean, well-scoped perf change: it caches PropertyChanged/PropertyChanging event args by name in two static ConcurrentDictionarys and reuses them from the hot OnPropertyChanged/OnPropertyChanging paths, cutting per-notification allocations.
Points to resolve before merge
- (warning) Null property name throws.
GetOrAdd(propertyName, …)rejects null keys; raising a change notification with a null/empty name (the .NET "all properties changed" convention) would now throwArgumentNullExceptionwhere it previously didn't. Add a null guard. - (warning) Unbounded static-cache growth. Keyed by arbitrary strings off the public API, the cache can grow without bound for dynamically-named properties (indexers, generated names). Consider bounding it or restricting caching to known
BindablePropertynames. (3/4 models flagged this; gemini rated it higher.)
Otherwise the optimization is sound and tests/CI are green.
| private static readonly ConcurrentDictionary<string, PropertyChangingEventArgs> s_changingArgsCache = new(); | ||
|
|
||
| internal static PropertyChangedEventArgs GetCachedPropertyChangedEventArgs(string propertyName) | ||
| => s_changedArgsCache.GetOrAdd(propertyName, static name => new PropertyChangedEventArgs(name)); |
There was a problem hiding this comment.
ConcurrentDictionary<string,T>.GetOrAdd(propertyName, ...) throws ArgumentNullException when propertyName is null. OnPropertyChanged/OnPropertyChanging are protected virtual and, by .NET convention, can be raised with a null/empty property name to signal "all properties changed". Before this change that path was harmless; now it would throw. Please null-guard propertyName before the cache lookup (e.g. fall back to new PropertyChangedEventArgs(propertyName) when null).
|
|
||
| internal ValidateValueDelegate ValidateValue { get; private set; } | ||
|
|
||
| private static readonly ConcurrentDictionary<string, PropertyChangedEventArgs> s_changedArgsCache = new(); |
There was a problem hiding this comment.
These caches are static (app-lifetime) and keyed by arbitrary strings flowing through the public OnPropertyChanged/OnPropertyChanging surface. For dynamically-named properties (e.g. indexer notifications like Item[0], Item[1], … or generated names) this grows unboundedly for the life of the process. Consider bounding the cache, or only caching for known BindableProperty.PropertyName values rather than arbitrary strings. (Raised by 3 of the 4 review models.)
MauiBot
left a comment
MauiBot
left a comment
There was a problem hiding this comment.
Expert Review — 2 findings
See inline comments for details.
| internal static PropertyChangedEventArgs GetCachedPropertyChangedEventArgs(string propertyName) | ||
| => s_changedArgsCache.GetOrAdd(propertyName, static name => new PropertyChangedEventArgs(name)); | ||
|
|
||
| internal static PropertyChangingEventArgs GetCachedPropertyChangingEventArgs(string propertyName) |
There was a problem hiding this comment.
[major] Memory Leak Prevention / Performance — These static caches are populated through the protected BindableObject.OnPropertyChanged(string) / OnPropertyChanging(string) hooks, so app or derived-control code can pass arbitrary property-name strings. Dynamic names such as indexer notifications (Item[123]) or generated names would now be retained for the app lifetime, whereas before the event args were short-lived. Since the optimization only needs known BindableProperty.PropertyName notifications, please scope caching to those known properties or avoid caching arbitrary protected-API strings.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
The concern about indefinitely caching event args for properties that only fire once makes sense as a follow-up tradeoff to keep an eye on. I don't think we should complicate this PR with eviction yet unless we see evidence that one-shot property names meaningfully accumulate in real apps. The current change keeps the implementation simple and removes allocation churn for the common hot paths. If this does show up as retained memory in profiling, two reasonable follow-up designs would be:
A time window on the order of tens of milliseconds, e.g. ~50ms, might be enough to cover bursty UI activity without keeping rare entries indefinitely, but I would prefer to validate that with allocation/retention data before adding the extra complexity. |
github-actions
Bot
added
the
s/agent-ready-for-rerun
github-actions
Bot
removed
the
s/agent-ready-for-rerun
This comment has been minimized.
This comment has been minimized.
github-actions
Bot
added
the
s/agent-review-in-progress
This comment has been minimized.
This comment has been minimized.
MauiBot
removed
the
s/agent-review-in-progress
A recurring android/mac gate failure is DotNet.csproj MSB6003 'System.IO.Pipes.dll ... cannot find the file': the shared Microsoft.NETCore.App pack was provisioned WITHOUT System.IO.Pipes.dll (an assembly that always ships in that framework). 'dotnet --version' still succeeds (it never loads that assembly) and the SDK carries a valid cake stamp, so cake REUSES the corrupt SDK and every build that spawns a process (Exec 'sh') dies. On the GATE stage this blocks the whole review: setup never completes, the sentinel is never written, and NO review is posted. Observed on #34136 (build 14753151 — last review was June 20, this build posted nothing) and #36657. The existing 'Prepare .dotnet' step can't catch it: it is windows-only and its --version probe can't see a missing runtime assembly. Add a non-windows, best-effort (continueOnError) pre-step that checks each installed Microsoft.NETCore.App pack for System.IO.Pipes.dll and, if missing, removes ./.dotnet so the build step reinstalls a complete SDK. It only ever deletes a provably-incomplete SDK and can never fail the build itself. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 15d2af20-e4ab-4e88-9011-cfbd83513bc0
This comment has been minimized.
This comment has been minimized.
In deferred posting, when the gate/review agent produced no PRAgent content (e.g. the Review stage failed before uploading its logs), the 'no PRAgent dir' branch only LOGGED 'falling back to posting deep results only' and posted NOTHING — the actual posting lives entirely in the else branch. So a build that ran the full deep suite could leave the author with zero feedback (observed on #34136 build 14755367: deep succeeded, review never posted). Post a standalone deep-results review in that branch so the deep outcome is always surfaced, with an honest note that the full AI summary didn't generate (infra) and to re-comment /review. Best-effort (try/catch + LASTEXITCODE reset) so it can never fail the Post task; scoped entirely to the previously-no-op branch so the normal (PRAgent-present) path is untouched. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 15d2af20-e4ab-4e88-9011-cfbd83513bc0
…ults post Per feedback on #34136: when a review stage fails, the PR was left showing only the bare 'Automated review could not complete — no review summary was produced' notice, even though the deep UI tests ran. The deferred-post fallback (385d70d) posts a standalone deep-results review in that case, but it did NOT collapse the earlier review-incomplete notice — so the PR ended with BOTH a real deep-results review AND a contradictory 'no summary produced' warning. After the standalone deep-results review posts, collapse the review-incomplete notice (Hide-StaleMauiBotIssueComments -IncludeReviewIncomplete) so the PR is FINISHED with an actual summary — matching what the normal post path already does. Best-effort; never fatal. When there are genuinely no deep results either, the notice is left as the honest outcome. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 15d2af20-e4ab-4e88-9011-cfbd83513bc0
This comment has been minimized.
This comment has been minimized.
github-actions
Bot
added
the
s/agent-review-in-progress
This comment has been minimized.
This comment has been minimized.
MauiBot
left a comment
MauiBot
left a comment
There was a problem hiding this comment.
AI Review Summary
ℹ️ The review agent did not produce a full summary on this run (an infrastructure issue on the CI agent), but the deep UI tests completed — their results are below. Re-comment
/reviewfor a fresh full review.
✅ Deep UI tests — 354 passed, 0 failed across 3 categories on platform-pool agent (replaces in-process counts above).
🧪 UI Test Execution Results (deep, platform pool)
| Category | Tests | Snapshot diffs |
|---|---|---|
Button |
67/69 ✓ | 1 diff PNG |
Label |
96/98 ✓ | — |
Layout |
191/194 ✓ | — |
📎 Download drop-deep-uitests artifact (TRX + snapshot diffs) |
MauiBot
removed
the
s/agent-review-in-progress
This comment has been minimized.
This comment has been minimized.
github-actions
Bot
added
the
s/agent-review-in-progress
MauiBot
left a comment
MauiBot
left a comment
There was a problem hiding this comment.
Expert Review — 6 findings
See inline comments for details.
| => s_changingArgsCache.GetOrAdd(propertyName, static name => new PropertyChangingEventArgs(name)); | ||
|
|
||
| // Properties that this property depends on - when getting this property's value, | ||
| // if the dependency has a pending binding, return the default value instead. |
There was a problem hiding this comment.
🔍 AI-Generated Review (multi-model)
[critical] Logic and Correctness — ConcurrentDictionary<string,T>.GetOrAdd throws ArgumentNullException when the key is null, so a null propertyName now crashes where it previously worked. BindableObject.OnPropertyChanged(string propertyName = null) / OnPropertyChanging(string propertyName = null) are protected virtual with a null default, and derived overrides forward a nullable value straight to base (e.g. Border.cs:443, BoxView.cs:150, RefreshView.cs:187, RadioButton.cs:377 all call base.OnPropertyChanged(propertyName) with string?). Concrete scenario: a subclass (in-tree or third-party) calls OnPropertyChanged(null) — the documented INotifyPropertyChanged idiom meaning "all properties changed" — or forwards a null variable; before this change new PropertyChangedEventArgs(null) was valid and the event was raised, now the call throws. The same applies to GetCachedPropertyChangingEventArgs on line 269. Guard the null case (return a cached args instance built for null, or fall back to new PropertyChangedEventArgs(propertyName) when propertyName is null).
| internal static PropertyChangedEventArgs GetCachedPropertyChangedEventArgs(string propertyName) | ||
| => s_changedArgsCache.GetOrAdd(propertyName, static name => new PropertyChangedEventArgs(name)); | ||
|
|
||
| internal static PropertyChangingEventArgs GetCachedPropertyChangingEventArgs(string propertyName) |
There was a problem hiding this comment.
🔍 AI-Generated Review (multi-model)
[major] Memory Leak Prevention — These are process-lifetime static caches with no eviction and no bound, keyed on an arbitrary caller-supplied string. OnPropertyChanged/OnPropertyChanging are public-surface (protected virtual) entry points, so any BindableObject subclass that raises change notifications with a dynamically composed name — indexer-style names such as $"Item[{i}]", names derived from collection items, or per-instance generated names — permanently adds one dictionary entry plus one PropertyChangedEventArgs/PropertyChangingEventArgs per distinct string, and neither the entry nor the string is ever released even after every object using it is collected. Unlike the previous per-call allocation (gen0, collected immediately), this converts a transient allocation into an unbounded gen2-rooted one. If the intent is to cache only the framework's fixed set of names, key the cache off the BindableProperty instance (which already owns a stable PropertyName) or store the args in a field on BindableProperty, and allocate normally for names that do not come from a registered property.
| internal static PropertyChangingEventArgs GetCachedPropertyChangingEventArgs(string propertyName) | ||
| => s_changingArgsCache.GetOrAdd(propertyName, static name => new PropertyChangingEventArgs(name)); | ||
|
|
||
| // Properties that this property depends on - when getting this property's value, |
There was a problem hiding this comment.
🔍 AI-Generated Review (multi-model)
[moderate] Performance-Critical Path — This replaces a small gen0 allocation on the property-change path with a string hash plus ConcurrentDictionary lookup on every notification, which is not obviously a win: PropertyChangedEventArgs is a 2-field object that the gen0 allocator handles in a few instructions, while GetOrAdd costs a full string hash (proportional to name length) plus a bucket probe and comparison. A cheaper design exists for the dominant call path — BindableObject.SetValue raises notifications with property.PropertyName, so the args instance could be stored in a field on the BindableProperty itself and reused with zero lookup. Since the change is justified purely on performance, it needs measured evidence (dotnet-trace / BenchmarkDotNet on a property-change-heavy scenario) showing the dictionary lookup beats the allocation it removes; otherwise this is a behavior-changing rewrite of a hot path with no proven benefit.
MauiBot
left a comment
MauiBot
left a comment
There was a problem hiding this comment.
AI Review Summary
@simonrozsival — new AI review results are available based on commit
e3e616a.
🗂️ Review Sessions — click to expand
🚦 Gate — Test Before & After Fix
Gate Result: ⚠️ SKIPPED
No tests were detected in this PR.
Recommendation: Add tests to verify the fix using the write-tests-agent.
📋 Pre-Flight — Context & Validation
Issue: #34092 - [Perf] Cache PropertyChangedEventArgs and PropertyChangingEventArgs on BindableProperty
PR: #34136 - [Perf] Cache PropertyChanged/ChangingEventArgs on BindableProperty
Base / Head: net11.0 / squashed review commit 249f0bcdc8
Requested Platform: Mac Catalyst (catalyst)
Files Changed: 2 implementation, 0 test
Gate: SKIPPED - no tests detected in the PR; the gate was not re-run.
Current PR Approach
BindableObject.OnPropertyChanged(string) and OnPropertyChanging(string) preserve their existing virtual dispatch but obtain event arguments from two static ConcurrentDictionary<string, TEventArgs> caches on BindableProperty. The change removes repeated event-argument allocations for repeated property names.
diff --git a/src/Controls/src/Core/BindableObject.cs b/src/Controls/src/Core/BindableObject.cs
@@
- => PropertyChanged?.Invoke(this, new PropertyChangedEventArgs(propertyName));
+ => PropertyChanged?.Invoke(this, BindableProperty.GetCachedPropertyChangedEventArgs(propertyName));
@@
- => PropertyChanging?.Invoke(this, new PropertyChangingEventArgs(propertyName));
+ => PropertyChanging?.Invoke(this, BindableProperty.GetCachedPropertyChangingEventArgs(propertyName));
diff --git a/src/Controls/src/Core/BindableProperty.cs b/src/Controls/src/Core/BindableProperty.cs
@@
+ private static readonly ConcurrentDictionary<string, PropertyChangedEventArgs> s_changedArgsCache = new();
+ private static readonly ConcurrentDictionary<string, PropertyChangingEventArgs> s_changingArgsCache = new();
+
+ internal static PropertyChangedEventArgs GetCachedPropertyChangedEventArgs(string propertyName)
+ => s_changedArgsCache.GetOrAdd(propertyName, static name => new PropertyChangedEventArgs(name));
+
+ internal static PropertyChangingEventArgs GetCachedPropertyChangingEventArgs(string propertyName)
+ => s_changingArgsCache.GetOrAdd(propertyName, static name => new PropertyChangingEventArgs(name));Key Findings and Constraints
ConcurrentDictionary.GetOrAdd(null, ...)throwsArgumentNullException; previously the protected virtual notification methods acceptednull, the standard "all properties changed" notification.- The static dictionaries retain arbitrary names supplied through protected virtual APIs for process lifetime. Dynamic/indexer-like names can therefore cause unbounded retained memory.
- Earlier review findings about bypassing
OnPropertyChanged(string)/OnPropertyChanging(string)overrides apply to an older revision and are fixed in the current patch. - The issue proposed caching on each
BindableProperty, but the current PR caches globally by arbitrary string. Alternative candidates should preserve virtual dispatch, null-name behavior, and bounded retention while still removing allocations from theSetValuehot path. - No public API surface is intentionally added.
Prior Discussion
- Reviewers repeatedly requested a null guard and restriction or bounding of the global cache.
- The author preferred not to add LRU/expiry complexity without retention measurements.
- Existing CI was previously reported green, but no PR test directly verifies allocation reuse, null names, or bounded retention.
Targeted Validation
Primary and regression test command:
dotnet test src/Controls/tests/Core.UnitTests/Controls.Core.UnitTests.csproj --filter "FullyQualifiedName~BindableObjectUnitTests|FullyQualifiedName~BindablePropertyUnitTests"Do not run the gate or a full test suite. The existing test project is platform-neutral; catalyst remains the requested target-platform context.
Fix Candidates
| # | Source | Approach | Test Result | Files Changed | Notes |
|---|---|---|---|---|---|
| PR | PR #34136 | Static dictionaries keyed by every notification name | SKIPPED (gate found no tests) | BindableObject.cs, BindableProperty.cs |
Null-name exception and unbounded retention unresolved |
🔬 Code Review — Deep Analysis
Code Review — PR #34136
Independent Assessment
What this changes: Replaces per-notification PropertyChangedEventArgs and PropertyChangingEventArgs allocations in the protected virtual BindableObject notification methods with two process-wide ConcurrentDictionary<string, ...> caches hosted on BindableProperty.
Inferred motivation: Reduce allocation churn on the SetValue and ClearValue hot paths while preserving existing virtual dispatch.
Approach assessment: The current implementation is not safe to merge. It fixes the earlier override-bypass design, but it broadens caching from the finite set of BindableProperty instances to every arbitrary string supplied through protected APIs. That creates a concrete null-name regression and app-lifetime retention of unbounded caller-provided names. A cache scoped to the active BindableProperty, with uncached fallback behavior for direct protected notifications, better matches the optimization target.
Reconciliation with PR Narrative
Author claims: Event args are cached on each BindableProperty through lazy instance fields, and SetValueCore/ClearValue use new event-args overloads.
Agreement/disagreement: The motivation is consistent with the code, but the description is stale. The submitted HEAD instead uses static dictionaries keyed by property-name strings and does not add the described overloads or per-property fields. Its benchmark therefore does not establish the performance of the submitted dictionary implementation.
Prior Review Reconciliation
| Prior finding | Source | Status | Evidence |
|---|---|---|---|
| New event-args overloads bypass protected virtual notification hooks | Copilot inline review | ✅ Fixed | Current SetValue/ClearValue paths still call OnPropertyChanged(string) and OnPropertyChanging(string). |
Null notification names throw in ConcurrentDictionary.GetOrAdd |
kubaflo and MauiBot | ❌ Unresolved | BindableProperty.cs:266,269 pass the nullable protected-method argument directly as a dictionary key. |
| Static caches retain arbitrary dynamic names for the app lifetime | pictos, kubaflo, and MauiBot | ❌ Unresolved | BindableProperty.cs:262-263 are static, unbounded dictionaries reached by protected virtual APIs. |
Blast Radius Assessment
- Runs for all instances: Yes. Every base
BindableObject.OnPropertyChangedandOnPropertyChangingcall uses the caches, not only frameworkSetValuenotifications. - Startup impact: No direct startup initialization; dictionaries initialize on first
BindablePropertytype use and populate on notifications. - Static/shared state: Yes. Both dictionaries are process-wide and retain keys and event args for the process lifetime.
- Platform scope: Shared Controls infrastructure; behavior affects every platform, including Mac Catalyst.
CI Status
- Required-check result: Current public check runs on live HEAD are completed successfully, including
maui-pr; authenticated required-check classification was unavailable. - Trusted Gate result:
⚠️ SKIPPED because the PR adds no tests. - Classification: Relevant regression coverage is undetermined. No test exercises null names, arbitrary dynamic names, or shared event-args reuse.
- Action taken: Confidence capped at low; Gate verification was not re-run.
Findings
❌ Error — Null notification names now throw
OnPropertyChanged(null) and OnPropertyChanging(null) previously constructed valid event args and raised the event. They now reach ConcurrentDictionary.GetOrAdd(null, ...), which throws ArgumentNullException. These protected virtual methods explicitly default to null and derived controls forward nullable values to base, so this is a concrete public-surface regression.
⚠️ Warning — Process-wide caches have an unbounded caller-controlled key space
The protected notification methods accept arbitrary names, including generated indexer names. Each distinct string is now rooted by both static dictionaries for the app lifetime, converting transient allocations into permanent retention.
⚠️ Warning — Hot-path benefit of dictionary lookup is not established
Every notification now pays string hashing and concurrent dictionary lookup. The supplied benchmark describes a different per-BindableProperty implementation, so it does not demonstrate that the submitted implementation improves the hot path.
⚠️ Warning — No regression coverage accompanies the behavior change
The PR adds no tests for null and empty names, repeated reuse, concurrent raises, or dynamic-name retention.
💡 Discussion — Event-args identity becomes globally shared
Subscribers previously received a fresh args object per raise. The PR intentionally changes that observable identity and lifetime but does not document the compatibility tradeoff.
Failure-Mode Probing
- A subclass calls
OnPropertyChanged(null): the dictionary rejects the key before any event is raised. - A custom control emits
$"Item[{index}]"names over time: both static dictionaries retain every distinct string and args object after the controls are collected. - Multiple objects raise the same property concurrently: immutable args are safe to share, but all callers pay the concurrent lookup and observe globally shared identity.
- A derived override forwards to
base: virtual dispatch is preserved in the current revision, but nullable forwarding still reaches the null-key exception.
External Output Contract
Not applicable.
Verdict: NEEDS_CHANGES
Confidence: low
Summary: The null-key crash is a verified regression and the unbounded static retention is unresolved. Because this changes shared notification infrastructure and the trusted Gate detected no tests, the safety recommendation remains low-confidence even though the defects themselves are concrete.
🛠️ Try-Fix — Analysis & Comparison
Try-Fix Aggregate — PR #34136
Bound: at most two candidates; sequential single-pass attempts.
Gate: not re-run (already skipped because the PR adds no tests).
Targeted command: dotnet test src/Controls/tests/Core.UnitTests/Controls.Core.UnitTests.csproj --filter "FullyQualifiedName~BindableObjectUnitTests|FullyQualifiedName~BindablePropertyUnitTests"
| # | Model | Approach | Result | Test | Diff | Self-review |
|---|---|---|---|---|---|---|
| 1 | claude-opus-5 |
Cache event args on each BindableProperty; pass the active property through a scoped, re-entrancy-safe notification hint while retaining virtual string hooks |
Blocked | Not run: baseline was not established | Empty | 0 findings |
| 2 | gpt-5.6-sol |
Replace strong dictionaries with weak-key ConditionalWeakTable caches keyed by notification-string identity, with an explicit null path |
Blocked | Not run: baseline was not established | Empty | 0 findings |
Candidate 1 — Per-BindableProperty Cache with Scoped Notification Hint
Result: Blocked
The proposed mechanism moves cache identity from arbitrary caller-controlled strings in process-wide dictionaries to the owning BindableProperty. Each property would lazily publish immutable changed/changing event args, while a save/restore hint around the existing virtual call would let the base notifier use those args only for framework SetValue notifications. Null and arbitrary names would retain their original collectable-allocation behavior, declared-property retention would remain bounded, and override dispatch would be preserved.
This approach was not applied. EstablishBrokenBaseline.ps1 rejected 44 pre-existing unrelated tracked worktree modifications before creating .github/.baseline-state.json. Without a RevertedFiles allow-list, the skill contract prohibited editing either PR file. The exact targeted test was therefore not run, fix.diff is empty, and the script-only restore returned the permitted no-state blocked result (Restored False, no attempt edits made). Full narrative: ../try-fix-1/content.md; mandatory artifacts: attempt-1/.
Candidate 2
Weak-Key Cache by Notification-String Identity
Result: Blocked
This distinct mechanism would replace the PR's strong ConcurrentDictionary caches with ConditionalWeakTable<string, TEventArgs> caches. The existing protected virtual string methods would remain the event-raising path. A direct null branch would preserve the all-properties notification, while non-null names would be cached by object identity. Normal SetValue calls repeatedly pass the stable BindableProperty.PropertyName string instance and therefore reuse event args; caller-created names would not be kept alive by the cache, and key/value cycles remain collectible under ephemeron semantics.
This approach uses neither candidate 1's per-property fields nor its scoped current-property hint. It was not applied for the same independent environment blocker: the mandatory baseline script rejected the 44 pre-existing unrelated tracked modifications and created no restoration state or edit allow-list. The targeted test was not run, fix.diff is empty, self-review recorded 0 findings, and the script-only restore returned the permitted no-state blocked result (Restored False, no attempt edits made). Full narrative: ../try-fix-2/content.md; mandatory artifacts: attempt-2/.
Aggregate Outcome
Two mechanism-level alternatives were produced, but neither could be implemented or empirically tested because the mandatory baseline workflow refuses the pre-existing dirty worktree before isolating the two PR files. Both results are therefore Blocked, not Pass; no source or test file was changed, and no gate or full suite was run.
📝 PR Finalize — Recommended Title & Description
Assessment: ✏️ Recommend updating — the current description documents an earlier per-BindableProperty/overload implementation, but the submitted HEAD uses process-wide dictionaries keyed by property name through the existing virtual methods.
Recommended title
[Core] BindableObject: Cache property notification event args by name
Recommended description
Fixes https://github.com/dotnet/maui/issues/34092
### Description of Change
Every base `BindableObject.OnPropertyChanged` and `OnPropertyChanging` call allocates a new `PropertyChangedEventArgs` or `PropertyChangingEventArgs`. This change caches those immutable event args by property name so repeated notifications can reuse them.
### Changes
- **`BindableProperty.cs`** — adds two process-wide `ConcurrentDictionary<string, PropertyChangedEventArgs/PropertyChangingEventArgs>` caches and internal lookup methods.
- **`BindableObject.cs`** — updates the existing protected virtual `OnPropertyChanged(string)` and `OnPropertyChanging(string)` methods to retrieve cached event args while preserving virtual dispatch.
### Behavioral Impact
Event-args instances are shared across `BindableObject` instances for the same property-name string instead of being newly allocated for every notification. The cache applies to every name passed through the protected virtual notification methods, not only names originating from a registered `BindableProperty`.
🏁 Report — Final Recommendation
⚠️ Final Recommendation: REQUEST CHANGES
Winner: pr-plus-reviewer
The submitted PR has a concrete regression: OnPropertyChanged(null) and OnPropertyChanging(null) now throw because ConcurrentDictionary rejects null keys. Its process-wide caches also retain every arbitrary notification name for the app lifetime, and the trusted Gate was skipped because the PR adds no tests.
| Candidate | Implementation | Validation | Assessment |
|---|---|---|---|
pr-plus-reviewer |
Per-BindableProperty lazy event args, scoped through the existing virtual string hooks; direct null/empty/arbitrary notifications preserve prior behavior; focused regression tests added |
PASS: 111 passed, 0 failed, 0 skipped | Winner. Resolves the verified null regression and unbounded caller-controlled cache while retaining the intended hot-path reuse and virtual dispatch. |
pr |
Two process-wide ConcurrentDictionary<string, ...> caches used by every base notification |
Gate SKIPPED; current public CI checks are green, but no relevant PR tests were added | Not acceptable as submitted because the null-name crash and unbounded retention remain. |
try-fix-1 |
Proposed per-BindableProperty cache with a scoped notification hint |
BLOCKED: no patch and no test run | Sound mechanism and closest to the winner, but it was never materialized or validated. |
try-fix-2 |
Proposed weak-key ConditionalWeakTable caches keyed by string identity with a null fallback |
BLOCKED: no patch and no test run | Avoids strong-key retention conceptually, but it was never materialized or validated and retains a lookup on every notification. |
Why pr-plus-reviewer wins
It is the only candidate that both addresses the expert review's blocking correctness and retention findings and passes the required targeted regression suite. It also preserves the public protected virtual hooks, confines cached lifetime to declared BindableProperty instances, keeps direct caller-supplied names collectable, and removes dictionary hashing from the framework SetValue/ClearValue path.
The raw PR cannot be approved because the winning changes are not present in the submitted HEAD. The two STEP 5a candidates rank below the passing candidate because both were blocked before producing code or regression evidence.
📱 UI Tests — Button,Label,Layout
Detected UI test categories: Button,Label,Layout
❌ Deep UI tests — 23 passed, 313 failed, 10 skipped across 3 categories on platform-pool agent (replaces in-process counts above).
🧪 UI Test Execution Results (deep, platform pool)
| Category | Tests | Snapshot diffs |
|---|---|---|
Button |
2/70 (67 ❌, 1 skipped) | 16 diff PNGs |
Label |
10/86 (71 ❌, 5 skipped) | 13 diff PNGs |
Layout |
11/190 (175 ❌, 4 skipped) | 6 diff PNGs |
🔍 AI analysis of failures — PR-related vs unrelated
🔍 AI-generated triage (GitHub Copilot CLI) — a heuristic judgement of whether each deep UI test failure is connected to this PR's changes. Verify before relying on it.
Likely unrelated: the failures appear pre-existing, flaky, or infrastructure.
- ● Unrelated — Appium element and interaction timeouts across Button, Label, and Layout (300+ tests): the repeated element-wait and 45-second Tap/Click command timeouts are run-wide driver or app-responsiveness failures, with no stack path through the changed event-notification code.
- ● Unrelated — Catalyst visual baseline and comparison-harness failures (at least 2 tests): the 2560x1571 actual image versus 789x563 baseline and the unparseable difference result indicate window-size, snapshot-baseline, or visual-harness issues rather than changed
BindableObjectbehavior.
Strongest signal: unrelated controls fail with the same Appium timeout signature, while the only concrete visual mismatch shows a drastically different Catalyst capture size.
❌ Button — 67 failed tests
ButtonImageTintColorPreservedAfterResize
System.TimeoutException : An Appium command did not complete within 45s. The application may be unresponsive (e.g., due to an infinite layout loop).
at UITest.Appium.HelperExtensions.RunWithTimeout[T](Func`1 action, Nullable`1 timeout) in /_/src/TestUtils/src/UITest.Appium/HelperExtensions.cs:line 3084
at UITest.Appium.HelperExtensions.RunWithTimeout(Action action, Nullable`1 timeout) in /_/src/TestUtils/src/UITest.Appium/HelperExtensions.cs:line 3103
at UITest.Appium.HelperExtensions.Tap(IApp app, String element) in /_/src/TestUtils/src/UITest.Appium/HelperExtensions.cs:line 36
at Microsoft.Maui.TestCases.Tests.Issues.Issue25093.ButtonImageTintColorPreservedAfterResize() in /_/src/Controls/tests/TestCases.Shared.Tests/Tests/Issues/Issue25093.cs:line 18
at System.RuntimeMethodHandle.InvokeMethod(ObjectHandleOnStack target, Void** arguments, ObjectHandleOnStack sig, BOOL isConstructor, ObjectHandleOnStack result)
at System.Reflection.RuntimeMethodInfo.Invoke(Object obj, B
...
VerifyButtonPage7
System.TimeoutException : An Appium command did not complete within 45s. The application may be unresponsive (e.g., due to an infinite layout loop).
at UITest.Appium.HelperExtensions.RunWithTimeout[T](Func`1 action, Nullable`1 timeout) in /_/src/TestUtils/src/UITest.Appium/HelperExtensions.cs:line 3084
at UITest.Appium.HelperExtensions.RunWithTimeout(Action action, Nullable`1 timeout) in /_/src/TestUtils/src/UITest.Appium/HelperExtensions.cs:line 3103
at UITest.Appium.HelperExtensions.Tap(IApp app, String element) in /_/src/TestUtils/src/UITest.Appium/HelperExtensions.cs:line 36
at Microsoft.Maui.TestCases.Tests.Issues.Issue22306_3.VerifyButtonPage7() in /_/src/Controls/tests/TestCases.Shared.Tests/Tests/Issues/Issue22306_3.cs:line 82
at System.RuntimeMethodHandle.InvokeMethod(ObjectHandleOnStack target, Void** arguments, ObjectHandleOnStack sig, BOOL isConstructor, ObjectHandleOnStack result)
at System.Reflection.RuntimeMethodInfo.Invoke(Object obj, BindingFlags invokeA
...
Button_SetLineBreakModeWordWrap_VerifyVisualState
System.TimeoutException : Timed out waiting for element...
at UITest.Appium.HelperExtensions.Wait(Func`1 query, Func`2 satisfactory, String timeoutMessage, Nullable`1 timeout, Nullable`1 retryFrequency) in /_/src/TestUtils/src/UITest.Appium/HelperExtensions.cs:line 2785
at UITest.Appium.HelperExtensions.WaitForAtLeastOne(Func`1 query, String timeoutMessage, Nullable`1 timeout, Nullable`1 retryFrequency) in /_/src/TestUtils/src/UITest.Appium/HelperExtensions.cs:line 2812
at UITest.Appium.HelperExtensions.WaitForElement(IApp app, String marked, String timeoutMessage, Nullable`1 timeout, Nullable`1 retryFrequency, Nullable`1 postTimeout) in /_/src/TestUtils/src/UITest.Appium/HelperExtensions.cs:line 797
at Microsoft.Maui.TestCases.Tests.ButtonFeatureTests.Button_SetLineBreakModeWordWrap_VerifyVisualState() in /_/src/Controls/tests/TestCases.Shared.Tests/Tests/FeatureMatrix/ButtonFeatureTests.cs:line 379
at System.RuntimeMethodHandle.InvokeMethod(ObjectHandleOnStack target, Voi
...
ButtonsLayoutResolveWhenParentSizeChanges
System.InvalidOperationException : Unable to extract difference percentage from exception message.
at Microsoft.Maui.TestCases.Tests.UITest.VerifyScreenshot(String name, Nullable`1 retryDelay, Nullable`1 retryTimeout, Int32 cropLeft, Int32 cropRight, Int32 cropTop, Int32 cropBottom, Double tolerance, Boolean includeTitleBar) in /_/src/Controls/tests/TestCases.Shared.Tests/UITest.cs:line 299
at Microsoft.Maui.TestCases.Tests.Issues.Issue22306.ButtonsLayoutResolveWhenParentSizeChanges() in /_/src/Controls/tests/TestCases.Shared.Tests/Tests/Issues/Issue22306.cs:line 25
at System.RuntimeMethodHandle.InvokeMethod(ObjectHandleOnStack target, Void** arguments, ObjectHandleOnStack sig, BOOL isConstructor, ObjectHandleOnStack result)
at System.Reflection.RuntimeMethodInfo.Invoke(Object obj, BindingFlags invokeAttr, Binder binder, Object[] parameters, CultureInfo culture)
TextViewBasedControlsTextColorUpdates
System.TimeoutException : An Appium command did not complete within 45s. The application may be unresponsive (e.g., due to an infinite layout loop).
at UITest.Appium.HelperExtensions.RunWithTimeout[T](Func`1 action, Nullable`1 timeout) in /_/src/TestUtils/src/UITest.Appium/HelperExtensions.cs:line 3084
at UITest.Appium.HelperExtensions.RunWithTimeout(Action action, Nullable`1 timeout) in /_/src/TestUtils/src/UITest.Appium/HelperExtensions.cs:line 3103
at UITest.Appium.HelperExtensions.Tap(IApp app, String element) in /_/src/TestUtils/src/UITest.Appium/HelperExtensions.cs:line 36
at Microsoft.Maui.TestCases.Tests.Issues.Issue35513.TextViewBasedControlsTextColorUpdates() in /_/src/Controls/tests/TestCases.Shared.Tests/Tests/Issues/Issue35513.cs:line 18
at System.RuntimeMethodHandle.InvokeMethod(ObjectHandleOnStack target, Void** arguments, ObjectHandleOnStack sig, BOOL isConstructor, ObjectHandleOnStack result)
at System.Reflection.RuntimeMethodInfo.Invoke(Object obj, Bind
...
VerifyButtonPage3
System.TimeoutException : An Appium command did not complete within 45s. The application may be unresponsive (e.g., due to an infinite layout loop).
at UITest.Appium.HelperExtensions.RunWithTimeout[T](Func`1 action, Nullable`1 timeout) in /_/src/TestUtils/src/UITest.Appium/HelperExtensions.cs:line 3084
at UITest.Appium.HelperExtensions.RunWithTimeout(Action action, Nullable`1 timeout) in /_/src/TestUtils/src/UITest.Appium/HelperExtensions.cs:line 3103
at UITest.Appium.HelperExtensions.Tap(IApp app, String element) in /_/src/TestUtils/src/UITest.Appium/HelperExtensions.cs:line 36
at Microsoft.Maui.TestCases.Tests.Issues.Issue22306_3.VerifyButtonPage3() in /_/src/Controls/tests/TestCases.Shared.Tests/Tests/Issues/Issue22306_3.cs:line 42
at System.RuntimeMethodHandle.InvokeMethod(ObjectHandleOnStack target, Void** arguments, ObjectHandleOnStack sig, BOOL isConstructor, ObjectHandleOnStack result)
at System.Reflection.RuntimeMethodInfo.Invoke(Object obj, BindingFlags invokeA
...
SettingCharacterSpacingShouldNotCrash
System.TimeoutException : An Appium command did not complete within 45s. The application may be unresponsive (e.g., due to an infinite layout loop).
at UITest.Appium.HelperExtensions.RunWithTimeout[T](Func`1 action, Nullable`1 timeout) in /_/src/TestUtils/src/UITest.Appium/HelperExtensions.cs:line 3084
at UITest.Appium.HelperExtensions.RunWithTimeout(Action action, Nullable`1 timeout) in /_/src/TestUtils/src/UITest.Appium/HelperExtensions.cs:line 3103
at UITest.Appium.HelperExtensions.Tap(IApp app, String element) in /_/src/TestUtils/src/UITest.Appium/HelperExtensions.cs:line 36
at Microsoft.Maui.TestCases.Tests.Issues.Issue31238.SettingCharacterSpacingShouldNotCrash() in /_/src/Controls/tests/TestCases.Shared.Tests/Tests/Issues/Issue31238.cs:line 18
at System.RuntimeMethodHandle.InvokeMethod(ObjectHandleOnStack target, Void** arguments, ObjectHandleOnStack sig, BOOL isConstructor, ObjectHandleOnStack result)
at System.Reflection.RuntimeMethodInfo.Invoke(Object obj, Bind
...
Button_SetLineBreakModeHeadTruncation_VerifyVisualState
System.TimeoutException : Timed out waiting for element...
at UITest.Appium.HelperExtensions.Wait(Func`1 query, Func`2 satisfactory, String timeoutMessage, Nullable`1 timeout, Nullable`1 retryFrequency) in /_/src/TestUtils/src/UITest.Appium/HelperExtensions.cs:line 2785
at UITest.Appium.HelperExtensions.WaitForAtLeastOne(Func`1 query, String timeoutMessage, Nullable`1 timeout, Nullable`1 retryFrequency) in /_/src/TestUtils/src/UITest.Appium/HelperExtensions.cs:line 2812
at UITest.Appium.HelperExtensions.WaitForElement(IApp app, String marked, String timeoutMessage, Nullable`1 timeout, Nullable`1 retryFrequency, Nullable`1 postTimeout) in /_/src/TestUtils/src/UITest.Appium/HelperExtensions.cs:line 797
at Microsoft.Maui.TestCases.Tests.ButtonFeatureTests.Button_SetLineBreakModeHeadTruncation_VerifyVisualState() in /_/src/Controls/tests/TestCases.Shared.Tests/Tests/FeatureMatrix/ButtonFeatureTests.cs:line 328
at System.RuntimeMethodHandle.InvokeMethod(ObjectHandleOnStack targe
...
Button_SetLineBreakModeTailTruncation_VerifyVisualState
System.TimeoutException : Timed out waiting for element...
at UITest.Appium.HelperExtensions.Wait(Func`1 query, Func`2 satisfactory, String timeoutMessage, Nullable`1 timeout, Nullable`1 retryFrequency) in /_/src/TestUtils/src/UITest.Appium/HelperExtensions.cs:line 2785
at UITest.Appium.HelperExtensions.WaitForAtLeastOne(Func`1 query, String timeoutMessage, Nullable`1 timeout, Nullable`1 retryFrequency) in /_/src/TestUtils/src/UITest.Appium/HelperExtensions.cs:line 2812
at UITest.Appium.HelperExtensions.WaitForElement(IApp app, String marked, String timeoutMessage, Nullable`1 timeout, Nullable`1 retryFrequency, Nullable`1 postTimeout) in /_/src/TestUtils/src/UITest.Appium/HelperExtensions.cs:line 797
at Microsoft.Maui.TestCases.Tests.ButtonFeatureTests.Button_SetLineBreakModeTailTruncation_VerifyVisualState() in /_/src/Controls/tests/TestCases.Shared.Tests/Tests/FeatureMatrix/ButtonFeatureTests.cs:line 362
at System.RuntimeMethodHandle.InvokeMethod(ObjectHandleOnStack targe
...
This uitests section was shortened to keep every required review section visible. Full details remain available in the pipeline build artifacts.
🧭 Next Steps — reviewer changes required
The reviewer-enhanced candidate identified changes that are not yet in the submitted PR.
Why: The reviewer-refined PR fixes the submitted implementation's null-name crash and unbounded process-wide string retention while preserving virtual dispatch and per-BindableProperty hot-path reuse. It is the only materialized alternative to pass the required targeted suite: 111 passed, 0 failed.
Address the actionable findings in this review before merging.
MauiBot
added
s/agent-changes-requested
|
was looking at your PR description (#34155). And looking into the raws benchmarks, I notice that some of them have unreliable data. For example, this one
where for ChildCount the error is 161% . When Error is >= the Mean, that indicates that the result isn't reliable. The code here could really improve the performance, but it's against the wrong measure, so I would suggest to fix the benchmark before merging or doing further changes here. |
Note
Are you waiting for the changes in this PR to be merged?
It would be very helpful if you could test the resulting artifacts from this PR and let us know in a comment if this change resolves your issue. Thank you!
Fixes #34092
Description
Every
SetValuecall allocates newPropertyChangedEventArgsandPropertyChangingEventArgsobjects. SinceBindableProperty.PropertyNameis immutable, these are now cached on theBindablePropertyinstance via lazyfield ??=fields.Changes
BindableProperty.cs— two new cached fields:CachedPropertyChangedEventArgsandCachedPropertyChangingEventArgs, lazily initialized on first accessBindableObject.cs— new internalOnPropertyChanged(PropertyChangedEventArgs)andOnPropertyChanging(PropertyChangingEventArgs)overloads that accept pre-allocated args;SetValueCoreandClearValueupdated to use the cached argsBenchmark
4.5× faster, zero allocation for PropertyChanged args.